Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optimize the happy path in parXOrAccumulate #3370

Closed
wants to merge 2 commits into from

Conversation

kyay10
Copy link
Collaborator

@kyay10 kyay10 commented Feb 6, 2024

This is done by wrapping raised values in a FailureValue, and values of that class are never leaked outside, hence it always denotes that a value was raised.

@serras
Copy link
Member

serras commented Feb 6, 2024

This is all a bit magical to me, but let's start with some concrete questions: is it possible to make mightFail not inline? That way FailureValue wouldn't have to be exposed (even though it's internal, we still need to worry about future ABI compatibility).

@kyay10
Copy link
Collaborator Author

kyay10 commented Feb 7, 2024

Is this better? I hid Failure as a class entirely, and instead use a function that returns an Any?.
I know this is magical on the surface, but it's a very similar encoding to stdlib's Result, if you're familiar with that. This is also inspired by EmptyValue from arrow-core.

The idea is to use a union type A | Failure<Error>, and because Failure is our own private type, this is isomorphic to an Either<Error, A>. The only issue is that we lose our typing because Kotlin doesn't have unions (and hence we end up using Any?), so care is needed to ensure we don't get CCEs (or even worse, failing tests).

@nomisRev
Copy link
Member

nomisRev commented Feb 7, 2024

I'm a bit confused on the optimisation 🤔 To me initially it even seems that this is actually resulting in more allocations, or other performance implications.

I'm fine with a Result like encoding, we purposefully never choose it because it eliminates exhaustive uses of when which are a very popular usage in Kotlin.

I'm always a bit hesitant about performance improvements, unless it's extremely obvious like an isEmpty check beforehand, or an isComplete check to avoid a suspension point. Otherwise I feel it's a bit indecisive, or premature optimisation, etc without benchmarks.

I need to take a proper look at the code though, could you elaborate a bit your train of thought of the optimisation @kyay10? I think if we do decide to merge these, or similar changes, we need to document the optimisation similar as I read when studying the KotlinX Coroutines documentation.

@kyay10
Copy link
Collaborator Author

kyay10 commented Feb 11, 2024

For sure, let me clarify. When using our either builder, we end up allocating a wrapper regardless of whether the function completes successfully or raises a value. We also end up doing an is check when binding that value. In this PR, we use a mightFail builder that only allocates in the raise case. We still have an is check when binding, of course. The reason that this is safe to do is because our Failure class is never used or leaked elsewhere, and so checking for is Failure guarantees that we truly have a value that came from something that was raised. This is inspired by how Raise encodes its success and failure values. We end up basically with the same allocations in the raise case, but with less allocations in the normal-returning case.
In other words, what we have is that for some block Raise<E>.() -> A, we first assume that A == A & !Failure, which is true as long as the Arrow codebase itself doesn't call this function with such a value. Then, we construct an untagged Union A | Failure<Nel<E>>, which by the assumption should be equivalent to the same tagged union encoding of it (i.e. should be equivalent to an Either<A, Nel<E>>). Finally, we just bind it as you'd expect. The only tricky thing here is that Kotlin doesn't have unions, and so we have to use Any?, which means that we must be careful with our types.

@nomisRev nomisRev added the 2.0.0 Tickets / PRs belonging to Arrow 2.0 label Apr 14, 2024
@nomisRev
Copy link
Member

@kyay10 I labeled this as 2.0.0. We should move 2.0.0 into main asap, such that we can do a Beta/RC1 release.

@kyay10
Copy link
Collaborator Author

kyay10 commented Apr 14, 2024

Yep, that's fine! I will rebase/merge right when main becomes arrow2

@kyay10
Copy link
Collaborator Author

kyay10 commented Apr 23, 2024

Closed in favour of #3408 or perhaps a RaisingDeferred type.

@kyay10 kyay10 closed this Apr 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.0.0 Tickets / PRs belonging to Arrow 2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants